Conversation
There was a problem hiding this comment.
Pull Request Overview
Upgrades core dependencies (notably Express to v5) and rewrites route existence tests to inspect the Express routing table instead of making HTTP requests. Also updates Node.js engine requirement and adds a few repository file presence checks in tests.
- Upgrade dependencies (express 5.x, jest 30.x, mongodb 6.20.x, etc.) and Node engine to >=22.20.0
- Replace regex-string scanning in route tests with a routeExists helper, and add checks for repo files
- Modify maintenance middleware route in app.js
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| package.json | Bumps Node engine and several dependencies; removes @jest-mock/express; updates jest and supertest. |
| app.js | Changes the catch-all maintenance route path. |
| tests/routes_mounted.test.js | Replaces route detection logic with a helper function and adds file presence assertions. |
| * This is without middleware | ||
| */ | ||
| app.all('*', (req, res, next) => { | ||
| app.all('*_', (req, res, next) => { |
There was a problem hiding this comment.
The route path '_' will not match all requests; it will only match a literal path ending with an underscore. To preserve the intended catch-all behavior for maintenance gating, restore the path to '' so all routes are intercepted.
| app.all('*_', (req, res, next) => { | |
| app.all('*', (req, res, next) => { |
There was a problem hiding this comment.
NOT ANYMORE BECAUSE EXPRESS 5
| import fs from "fs" | ||
|
|
||
| let app_stack = app._router.stack | ||
| let app_stack = app.router.stack |
There was a problem hiding this comment.
Express exposes the router stack on app._router, not app.router. app.router is undefined in Express 4 and 5, which will cause a TypeError when accessing .stack. Use app._router.stack instead.
| let app_stack = app.router.stack | |
| let app_stack = app._router.stack |
There was a problem hiding this comment.
NOT ANYMORE BECAUSE EXPRESS 5
| function routeExists(stack, testPath) { | ||
| for (const layer of stack) { | ||
| // Check if layer has matchers (Express 5) | ||
| if (layer.matchers && layer.matchers.length > 0) { | ||
| const matcher = layer.matchers[0] | ||
| const match = matcher(testPath) | ||
| if (match && match.path) { | ||
| return true | ||
| } | ||
| } | ||
| // Also check route.path directly if it exists | ||
| if (layer.route && layer.route.path) { | ||
| if (layer.route.path === testPath || layer.route.path.includes(testPath)) { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Mount layers like '/v1' and '/client' are represented as Layer objects without layer.route, so this helper will miss mounted prefixes. Additionally, route.path can be an array. Add a fallback using layer.regexp.test(testPath) and handle array route paths to correctly detect mounts and multi-path routes.
There was a problem hiding this comment.
NOT ANYMORE BECAUSE EXPRESS 5
| "dependencies": { | ||
| "@jest/globals": "^29.7.0", | ||
| "cookie-parser": "~1.4.4", | ||
| "@jest/globals": "^30.2.0", |
There was a problem hiding this comment.
@jest/globals is only used in tests and should be listed under devDependencies instead of dependencies to avoid adding it to the production install footprint.
| "repository": "github:CenterForDigitalHumanities/rerum_server_nodejs", | ||
| "engines": { | ||
| "node": ">=22.12.0" | ||
| "node": ">=22.20.0" |
There was a problem hiding this comment.
The Node.js engine requirement has changed; please ensure the project's setup docs and any CI configuration are updated to reflect '>=22.20.0' so contributors and pipelines use a compatible version.
Includes updated exists tests.